Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Cache children in serializeComposeNode #1027

Merged
merged 1 commit into from
Jul 31, 2024
Merged

fix: Cache children in serializeComposeNode #1027

merged 1 commit into from
Jul 31, 2024
+3 −2

Conversation

idyatlov
Copy link
Contributor

@idyatlov idyatlov commented Jul 31, 2024 •
edited
Loading

We can have modifications in the nodes when traversing through them in serializeComposeNode, so it's important to cache children before printing them. Similar approach is used in compose-ui-test when printing compose nodes.

This is to prevent stack traces like that one 
        at jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
        at jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
        at jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
        at java.util.Objects.checkIndex(Objects.java:359)
        at java.util.ArrayList.get(ArrayList.java:434)
        at io.appium.espressoserver.lib.model.SourceDocument.serializeComposeNode(SourceDocument.java:244)
        at io.appium.espressoserver.lib.model.SourceDocument.toStream(SourceDocument.java:288)
        at io.appium.espressoserver.lib.model.SourceDocument.access$toStream(SourceDocument.java:87)
        at io.appium.espressoserver.lib.model.SourceDocument$findMatchingNodeIds$1.invoke(SourceDocument.java:376)
        at io.appium.espressoserver.lib.model.SourceDocument$findMatchingNodeIds$1.invoke(SourceDocument.java:375)
        at io.appium.espressoserver.lib.helpers.extensions.SemaphoreKt.withPermit(SemaphoreKt.java:17)
        at io.appium.espressoserver.lib.model.SourceDocument.findMatchingNodeIds(SourceDocument.java:375)
        at io.appium.espressoserver.lib.helpers.ComposeNodeFinderKt.hasXpath(ComposeNodeFinderKt.java:72)
        at io.appium.espressoserver.lib.helpers.ComposeNodeFinderKt.semanticsMatcherForLocator(ComposeNodeFinderKt.java:58)
        at io.appium.espressoserver.lib.helpers.ComposeNodeFinderKt.toNodeInteractionsCollection(ComposeNodeFinderKt.java:45)
        at io.appium.espressoserver.lib.handlers.FindElements.handleCompose(FindElements.java:44)
        at io.appium.espressoserver.lib.handlers.FindElements.handleCompose(FindElements.java:28)
        at io.appium.espressoserver.lib.handlers.RequestHandler$DefaultImpls.invokeStrategy(RequestHandler.java:34)
        at io.appium.espressoserver.lib.handlers.FindElements.invokeStrategy(FindElements.java:28)
        at io.appium.espressoserver.lib.handlers.FindElements.invokeStrategy(FindElements.java:28)
        at io.appium.espressoserver.lib.handlers.RequestHandler$DefaultImpls.handleInternal(RequestHandler.java:29)
        at io.appium.espressoserver.lib.handlers.FindElements.handleInternal(FindElements.java:28)
        at io.appium.espressoserver.lib.handlers.FindElements.handleInternal(FindElements.java:28)
        at io.appium.espressoserver.lib.handlers.RequestHandler$DefaultImpls.handle(RequestHandler.java:57)
        at io.appium.espressoserver.lib.handlers.FindElements.handle(FindElements.java:28)
        at io.appium.espressoserver.lib.handlers.FindElements.handle(FindElements.java:28)
        at io.appium.espressoserver.lib.http.Router.route(Router.java:232)
        at io.appium.espressoserver.lib.http.Server.serve(Server.java:78)
Some decompiled code to show the difference

Old code
Size is calculated once, but we calculate a new collection each time when getting a new child so if the collection has fewer elements there will be IndexOutOfBoundsException

for(int var15 = ((Collection)semanticsNode.getChildren()).size(); index < var15; ++index) {
   this.serializeComposeNode((SemanticsNode)semanticsNode.getChildren().get(index), depth + 1);
}

New code
we calculate children collection only once

List children = semanticsNode.getChildren();
int index = 0;
for(int var17 = ((Collection)children).size(); index < var17; ++index) {
   this.serializeComposeNode((SemanticsNode)children.get(index), depth + 1);
}

@idyatlov idyatlov changed the title Cache children in serializeComposeNode fix: Cache children in serializeComposeNode Jul 31, 2024
@mykola-mokhnach mykola-mokhnach merged commit eaff37d into appium:master Jul 31, 2024
11 of 12 checks passed
github-actions bot pushed a commit that referenced this pull request Jul 31, 2024
@semantic-release-bot
chore(release): 3.3.1 [skip ci]
bed533d 
## [3.3.1](v3.3.0...v3.3.1) (2024-07-31)

### Bug Fixes

* Cache children when traversing ([#1027](#1027)) ([eaff37d](eaff37d))
@github-actions GitHub Actions
Copy link

🎉 This issue has been resolved in version 3.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@KazuCocoa KazuCocoa added the size:S contribution size: S label Sep 6, 2024
@jlipps
Copy link
Member

jlipps commented Sep 6, 2024

Hi @idyatlov, congrats, the Appium project wants to compensate you for this contribution! Please reply to this comment mentioning me and sharing your OpenCollective account name, so that we can initiate payment! Or let me know if you decline to receive compensation via OpenCollective. Thank you!

@idyatlov
Copy link
Contributor Author

idyatlov commented Sep 7, 2024

Thanks @jlipps, my OpenCollective account name is ivan-dyatlov - and it will be my first compensation, I appreciate that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

Assignees
No one assigned
Labels
released size:S contribution size: S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@idyatlov @jlipps @mykola-mokhnach @KazuCocoa